HTML API: Ensure bookmark exaustion does not error by sirreal · Pull Request #10616 · WordPress/wordpress-develop

sirreal · 2025-12-10T16:37:55Z

Bookmark exhaustion, typically from deep nesting, can cause the HTML Processor to throw an Exception.

Rather than throwing an exception, return false when bookmark exhaustion is detected. An error is set on the processor and processing stops.

Trac ticket: https://core.trac.wordpress.org/ticket/64394

This Pull Request is for code review only. Please keep all other discussion in the Trac ticket. Do not merge this Pull Request. See GitHub Pull Requests for Code Review in the Core Handbook for more details.

sirreal · 2025-12-10T16:42:51Z

src/wp-includes/html-api/class-wp-html-processor.php

+	 * @throws Exception When unable to allocate a bookmark for the next token in the input HTML document.
+	 *


bookmark_token() actually throws, but it's not handled here. The annotation may not be appropriate.

sirreal · 2025-12-10T16:43:53Z

src/wp-includes/html-api/class-wp-html-processor.php

 			 * otherwise might involve messier calling and return conventions.
 			 */
 			return false;
+		} catch ( Exception $e ) {


Exhausted bookmarks throw a generic Exception.

This block catches the exceptions thrown by insert_virtual_token().

tests/phpunit/tests/html-api/wpHtmlProcessor.php

github-actions · 2025-12-10T17:03:30Z

Test using WordPress Playground

The changes in this pull request can previewed and tested using a WordPress Playground instance.

WordPress Playground is an experimental project that creates a full WordPress instance entirely within the browser.

Some things to be aware of

The Plugin and Theme Directories cannot be accessed within Playground.
All changes will be lost when closing a tab with a Playground instance.
All changes will be lost when refreshing the page.
A fresh instance is created each time the link below is clicked.
Every time this pull request is updated, a new ZIP file containing all changes is created. If changes are not reflected in the Playground instance,
it's possible that the most recent build failed, or has not completed. Check the list of workflow runs to be sure.

For more details about these limitations and more, check out the Limitations page in the WordPress Playground documentation.

Test this pull request with WordPress Playground.

src/wp-includes/html-api/class-wp-html-processor.php

Co-authored-by: Weston Ruter <westonruter@gmail.com>

src/wp-includes/html-api/class-wp-html-processor.php

github-actions · 2026-01-29T18:29:47Z

The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the props-bot label.

Core Committers: Use this line as a base for the props when committing in SVN:

Props jonsurrell, westonruter, dmsnell.

To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook.

Copilot

Pull request overview

This PR fixes an issue where bookmark exhaustion (typically from deep HTML nesting) causes the HTML Processor to throw an Exception. Instead of throwing exceptions, the processor now returns false when bookmark exhaustion is detected, sets an error state, and stops processing gracefully.

Changes:

Modified bookmark_token() to return false instead of throwing an exception when MAX_BOOKMARKS is exceeded
Updated all callers of bookmark_token() and insert_virtual_node() to check for and handle false return values
Removed outdated @throws documentation from methods that no longer throw exceptions
Added comprehensive tests to verify the processor handles extreme nesting without throwing exceptions

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.

File	Description
`src/wp-includes/html-api/class-wp-html-processor.php`	Core implementation changes: modified `bookmark_token()` and `insert_virtual_node()` to return `false` on failure instead of throwing; added error checks in `step()`, `step_before_html()`, `step_before_head()`, `step_after_head()`, `step_in_table()`, and `step_in_table_body()` methods; updated PHPDoc to remove exception references
`tests/phpunit/tests/html-api/wpHtmlProcessor.php`	Added two comprehensive tests verifying the processor handles bookmark exhaustion gracefully for both regular and virtual tokens; fixed annotation from `@group` to `@ticket`

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

dmsnell

I definitely don’t love how pervasive and deep our error-checking has become here; did you attempt to use ->bail()? the existing code is throwing because that part of step() isn’t wrapped in a try/catch, right? what if we continue to throw inside bookmark_token() but trap that and handle aborting the same way we handle all deep control flow issues?

sirreal · 2026-01-30T15:17:43Z

I definitely don’t love how pervasive and deep our error-checking has become here.

Me neither 😕

did you attempt to use ->bail()?

I did try using bail(). That transforms a bookmark exhaustion error into an unsupported error, which seemed confusing. When bookmarks are exhausted, an error is set. Bail overrides with an unsupported error code.

We could consider having bail leave the last error in case it's already been set.

what if we continue to throw inside bookmark_token() but trap that and handle aborting the same way we handle all deep control flow issues?

I have that in an earlier commit. It wasn't as pervasive (at b02d679):

wordpress-develop/src/wp-includes/html-api/class-wp-html-processor.php

Lines 1043 to 1050 in b02d679

    
           try { 
        
           	$bookmark_name = $this->bookmark_token(); 
        
           } catch ( Exception $e ) { 
        
           	if ( self::ERROR_EXCEEDED_MAX_BOOKMARKS === $this->last_error ) { 
        
           		return false; 
        
           	} 
        
           	throw $e; 
        
           }

wordpress-develop/src/wp-includes/html-api/class-wp-html-processor.php

Lines 1157 to 1169 in b02d679

    
           		} catch ( WP_HTML_Unsupported_Exception $e ) { 
        
           			/* 
        
           			 * Exceptions are used in this class to escape deep call stacks that 
        
           			 * otherwise might involve messier calling and return conventions. 
        
           			 */ 
        
           			return false; 
        
           		} catch ( Exception $e ) { 
        
           			if ( self::ERROR_EXCEEDED_MAX_BOOKMARKS === $this->last_error ) { 
        
           				return false; 
        
           			} 
        
           			// Rethrow any other exceptions for higher-level handling. 
        
           			throw $e; 
        
           		}

Co-authored-by: Weston Ruter <westonruter@gmail.com>

dmsnell · 2026-01-30T17:04:30Z

I have that in an earlier commit. It wasn't as pervasive

Why did you change it @sirreal? did I ask you to? 🙃 🤦‍♂️

sirreal · 2026-01-30T17:21:13Z

No, I was just iterating and exploring. The method actually already had a …|false return type annotation, suggesting it may have intended to return false on failure.

I can't say I like the general Exception catching either, but it works. I don't have a strong preference, would you prefer that other version @dmsnell?

westonruter

LGTM, although I defer to the expertise of @dmsnell.

dmsnell · 2026-01-31T17:33:56Z

@sirreal is there a copy of the exception-catching version available?

what do you not like about it, or prefer about adding |false to all of the methods?

sirreal · 2026-02-02T09:23:33Z

is there a copy of the exception-catching version available?

@dmsnell This diff should give an idea

what do you not like about it, or prefer about adding |false to all of the methods?

I don't feel strongly one way or the other.

Relying on error handling does reduce the checks required. My biggest concern is that it depends on methods being called inside of try/catch structures, which is easy to overlook. Handling a false return also requires some discipline, but at least the check is local and doesn't require understanding a call stack.

Some background to consider (all considering trunk):

bookmark_token() claims to have both behaviors (throw + return false) suggesting confusion or oversight. It's one or the other, currently throw.

wordpress-develop/src/wp-includes/html-api/class-wp-html-processor.php

Lines 5097 to 5107 in f4f33f7

    
           	/** 
        
           	 * Creates a new bookmark for the currently-matched token and returns the generated name. 
        
           	 * 
        
           	 * @since 6.4.0 
        
           	 * @since 6.5.0 Renamed from bookmark_tag() to bookmark_token(). 
        
           	 * 
        
           	 * @throws Exception When unable to allocate requested bookmark. 
        
           	 * 
        
           	 * @return string|false Name of created bookmark, or false if unable to create. 
        
           	 */ 
        
           	private function bookmark_token() {

insert_html_element() and insert_foreign_element() never fail because the token already exists with an allocated bookmark.

wordpress-develop/src/wp-includes/html-api/class-wp-html-processor.php

Lines 6242 to 6244 in f4f33f7

    
           private function insert_html_element( WP_HTML_Token $token ): void { 
        
           	$this->state->stack_of_open_elements->push( $token ); 
        
           }

insert_virtual_node() allocates a bookmark in order to construct a token. Virtual tokens are not "discovered" in the document, but arise from HTML parsing rules.

wordpress-develop/src/wp-includes/html-api/class-wp-html-processor.php

Lines 6295 to 6303 in f4f33f7

    
           private function insert_virtual_node( $token_name, $bookmark_name = null ): WP_HTML_Token { 
        
           	$here = $this->bookmarks[ $this->state->current_token->bookmark_name ]; 
        
           	$name = $bookmark_name ?? $this->bookmark_token(); 
        
           	$this->bookmarks[ $name ] = new WP_HTML_Span( $here->start, 0 ); 
        
           	$token = new WP_HTML_Token( $name, $token_name, false ); 
        
           	$this->insert_html_element( $token ); 
        
           	return $token;

The main thing I disliked was the general Exception catching that was required. It's possible for the last_error to be set and another Exception to arise:

wordpress-develop/src/wp-includes/html-api/class-wp-html-processor.php

Lines 1043 to 1050 in b02d679

    
           try { 
        
           	$bookmark_name = $this->bookmark_token(); 
        
           } catch ( Exception $e ) { 
        
           	if ( self::ERROR_EXCEEDED_MAX_BOOKMARKS === $this->last_error ) { 
        
           		return false; 
        
           	} 
        
           	throw $e; 
        
           }

wordpress-develop/src/wp-includes/html-api/class-wp-html-processor.php

Lines 1157 to 1169 in b02d679

    
           		} catch ( WP_HTML_Unsupported_Exception $e ) { 
        
           			/* 
        
           			 * Exceptions are used in this class to escape deep call stacks that 
        
           			 * otherwise might involve messier calling and return conventions. 
        
           			 */ 
        
           			return false; 
        
           		} catch ( Exception $e ) { 
        
           			if ( self::ERROR_EXCEEDED_MAX_BOOKMARKS === $this->last_error ) { 
        
           				return false; 
        
           			} 
        
           			// Rethrow any other exceptions for higher-level handling. 
        
           			throw $e; 
        
           		}

Perhaps that check should have compared the known message:

wordpress-develop/src/wp-includes/html-api/class-wp-html-processor.php

Line 5125 in b02d679

throw new Exception( 'could not allocate bookmark' );

I also considered introducing another HTML API Exception class, but I'm reluctant to introduce another exception class for this.

dmsnell · 2026-02-20T16:13:33Z

The exception-throwing and catching version definitely suits me more, notably because it avoids exporting these details to the callers.

I also think this is a middle-path to excluding internal bookmarks, so spreading out the protection throughout the class seems like it’s going to be code we then have to find and remove later (though maybe the recent PHPStan changes will help surface those cases).

which is easy to overlook

If our goal is to never fail under normal usage and the failed bookmarks are only there to prevent infinite loops or memory-abuse, then perhaps we add a test to ensure that setting the bookmark fails and also that it doesn’t throw.

sirreal · 2026-02-26T11:01:30Z

The exception-throwing and catching version definitely suits me more, notably because it avoids exporting these details to the callers.

I've restored the throw/catch version of these change.

perhaps we add a test to ensure that setting the bookmark fails and also that it doesn’t throw.

The PR includes tests like that. They test the processing behavior without testing the bookmark functions directly.

Copilot

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

dmsnell

As a stop-gap I’m approving this, but there are a number of things I am still wrestling with.

We can and I believe we should update the MAX_BOOKMARKS to 10,000 or possibly 6,000 (if we think we should be overly conservative, which I don’t). My approval stands if we want to update the count in this PR. When we limited the bookmarks in the HTML Processor we didn’t know what a good subjective value would be, but I believe that we’ve learned since then that we are safe to open it up to a point where we can parse all reasonable HTML documents on the internet, which is somewhere around 3,000 or 6,000 depending on whether we want to be able to parse clearly spam content or not.
This is highlighting other work you’ve done to separate the namespace of internal and external bookmarks, or of eliminating them.
The tests are hard to reason about, and they are making active assertions that the processor fails in tasks we want it to succeed in. I know we have to setup the test data in ways that should hit the failure point, but I don’t like knowing the fact that an update to fix the code will cause the tests to fail. This is not the same category for me as “we want to ensure nobody fixes this one piece without also fixing these other pieces” but more like we are writing a test that will bite us later.

Because I don’t want the tests to stand in the way I am giving the approval, but I would be happy as well if we either updated the tests to focus on the behaviors we want to enforce (that user-space bookmarking hits a limit), or even if we just removed the tests for now. I am nervous is all.

But yeah, please feel encouraged to bump the MAX_BOOKMARKS in this before merge unless you know a reason not to.

dmsnell · 2026-02-26T15:10:34Z

tests/phpunit/tests/html-api/wpHtmlProcessor.php

+			WP_HTML_Processor::ERROR_EXCEEDED_MAX_BOOKMARKS,
+			$processor->get_last_error(),
+			'Failed to report exceeded-max-bookmarks error.'
+		);


this test confuses me.

Not sure if you noticed, but we should have $this->assertEqualsWithDelta( MAX_BOOKMARKS, $reached_tokens, 1 ); available to say "within 1 of the max"

but why the variability? and what does “depending on how the tokens align” mean? it feels flakey or not-fully-understood to me, and I get nervous adding an assertion of that in case we’re testing a happenstance detail and not a contract.

This test is very tricky, and imperfect as it is. I want to confirm that these don't throw errors during processing, which is the big thing I want to address in this PR.

It would be fine to change this to assert that no assertions are thrown.

This test in particular was difficult because the virtual tokens had another failure path. Consider:

<table><td><table><td> produces:

└─TABLE (real) └─TBODY (virtual) └─TR (virtual) └─TD (real) └─TABLE (real) └─TBODY (virtual) └─TR (virtual) └─TD (real)

To make this test more robust, it would probably need to process 3 times:

<table><td>…

<div><table><td>…

<div><div><table><td>…

Since there are pairs of real tokens TD (real) + TABLE (real), running with 0, 1, 2 offsets should ensure that the virtual token lands on the threshold at least once.

tests/phpunit/tests/html-api/wpHtmlProcessor.php

sirreal · 2026-02-26T19:54:41Z

@dmsnell Thanks, I've reworked some of the testing based on your feedback.

I do plan to land #10820 to increase the max bookmarks as well.

When bookmark exhaustion occurs during processing, return `false` instead of throwing an `Exception`. Developed in #10616. Props jonsurrell, westonruter, dmsnell. Fixes #64394. git-svn-id: https://develop.svn.wordpress.org/trunk@61755 602fd350-edb4-49c9-b593-d223f7449a82

github-actions · 2026-02-27T13:06:16Z

A commit was made that fixes the Trac ticket referenced in the description of this pull request.

SVN changeset: 61755
GitHub commit: 7a58dc0

This PR will be closed, but please confirm the accuracy of this and reopen if there is more work to be done.

When bookmark exhaustion occurs during processing, return `false` instead of throwing an `Exception`. Developed in WordPress/wordpress-develop#10616. Props jonsurrell, westonruter, dmsnell. Fixes #64394. Built from https://develop.svn.wordpress.org/trunk@61755 git-svn-id: http://core.svn.wordpress.org/trunk@61061 1a063a9b-81f0-0310-95a4-ce76da25c4cd

sirreal added 7 commits December 10, 2025 13:59

Fix group -> ticket test annotation

2c9a17d

Add failing test for deep nesting

da53752

Handle Exception thrown by bookmark_token()

0337b13

Add failing test for virtual tokens

4101235

Handle insert_virtual_node exceptions

e7d649c

Fix php 7.x compat

1cfeb85

Update test ticket annotation

17bbca8

sirreal commented Dec 10, 2025

View reviewed changes

tests/phpunit/tests/html-api/wpHtmlProcessor.php Outdated Show resolved Hide resolved

Increase virtual token HTML size

ba480a5

westonruter reviewed Dec 11, 2025

View reviewed changes

src/wp-includes/html-api/class-wp-html-processor.php Show resolved Hide resolved

westonruter reviewed Dec 11, 2025

View reviewed changes

src/wp-includes/html-api/class-wp-html-processor.php Outdated Show resolved Hide resolved

Use single-line comment

ff7657f

Co-authored-by: Weston Ruter <westonruter@gmail.com>

sirreal commented Dec 12, 2025

View reviewed changes

src/wp-includes/html-api/class-wp-html-processor.php Show resolved Hide resolved

sirreal added 5 commits January 29, 2026 18:56

Merge branch 'trunk' into html-api/ensure-deep-nesting-no-exception

b02d679

Return false or throw unsupported exception where available

9e042f3

Remove general exception catching

c7710aa

Avoid throwing unsupported exception when tokens exceeded

1681fed

Merge branch 'trunk' into html-api/ensure-deep-nesting-no-exception

3f6bffd

sirreal mentioned this pull request Jan 29, 2026

HTML Processor: Increase MAX_BOOKMARKS #10820

Closed

Remove unsupported union return type

aefc5bf

sirreal marked this pull request as ready for review January 29, 2026 18:27

sirreal requested review from dmsnell and westonruter January 29, 2026 18:27

westonruter requested a review from Copilot January 29, 2026 21:44

Copilot started reviewing on behalf of westonruter January 29, 2026 21:44 View session

Copilot AI reviewed Jan 29, 2026

View reviewed changes

dmsnell reviewed Jan 30, 2026

View reviewed changes

Use arrow functions ✨

6d1f860

Co-authored-by: Weston Ruter <westonruter@gmail.com>

Merge branch 'trunk' into html-api/ensure-deep-nesting-no-exception

207f04f

Add assertion message and use class constant

cbab9c1

westonruter approved these changes Jan 30, 2026

View reviewed changes

Merge branch 'trunk' into html-api/ensure-deep-nesting-no-exception

c578125

sirreal added 2 commits February 26, 2026 11:42

Merge branch 'trunk' into html-api/ensure-deep-nesting-no-exception

5e45206

Restore exception-catching approach

c4476be

sirreal requested review from Copilot and dmsnell February 26, 2026 11:01

Copilot started reviewing on behalf of sirreal February 26, 2026 11:03 View session

Copilot AI reviewed Feb 26, 2026

View reviewed changes

dmsnell approved these changes Feb 26, 2026

View reviewed changes

sirreal added 2 commits February 26, 2026 20:52

Improve tests

b847559

Merge branch 'trunk' into html-api/ensure-deep-nesting-no-exception

aa06e5e

sirreal added 2 commits February 26, 2026 20:57

Lints

547d37b

Merge branch 'trunk' into html-api/ensure-deep-nesting-no-exception

b6df5f7

github-actions bot closed this Feb 27, 2026

sirreal deleted the html-api/ensure-deep-nesting-no-exception branch February 27, 2026 15:41

		* @throws Exception When unable to allocate a bookmark for the next token in the input HTML document.
		*

Conversation

sirreal commented Dec 10, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sirreal Dec 10, 2025

Choose a reason for hiding this comment

Uh oh!

sirreal Dec 10, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

github-actions bot commented Dec 10, 2025

Test using WordPress Playground

Some things to be aware of

Uh oh!

Uh oh!

Uh oh!

Uh oh!

github-actions bot commented Jan 29, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

dmsnell left a comment

Choose a reason for hiding this comment

Uh oh!

sirreal commented Jan 30, 2026

Uh oh!

dmsnell commented Jan 30, 2026

Uh oh!

sirreal commented Jan 30, 2026

Uh oh!

westonruter left a comment

Choose a reason for hiding this comment

Uh oh!

dmsnell commented Jan 31, 2026

Uh oh!

sirreal commented Feb 2, 2026

Uh oh!

dmsnell commented Feb 20, 2026

Uh oh!

sirreal commented Feb 26, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

dmsnell left a comment

Choose a reason for hiding this comment

Uh oh!

dmsnell Feb 26, 2026

Choose a reason for hiding this comment

Uh oh!

sirreal Feb 26, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

sirreal commented Feb 26, 2026

Uh oh!

github-actions bot commented Feb 27, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

sirreal commented Dec 10, 2025 •

edited

Loading

github-actions bot commented Jan 29, 2026 •

edited

Loading